Replace maxdict with lru_cache #52
Conversation
Requires an update to matplotlib-pyodide before it will work: pyodide/matplotlib-pyodide#52
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
======================================
Coverage ? 0.00%
======================================
Files ? 4
Lines ? 648
Branches ? 0
======================================
Hits ? 0
Misses ? 648
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| self.dpi = dpi | ||
| self.fontd = maxdict(50) | ||
| self.mathtext_parser = MathTextParser("bitmap") | ||
| self._get_font_helper = lru_cache(maxsize=50)(self._get_font_helper) |
There was a problem hiding this comment.
What's the difference between adding a lru_cache decorator on top of self._get_font_helper, or re-defining a variable like this?
There was a problem hiding this comment.
Two things:
- Decorating the method makes one shared cache across all class instances, this makes a per-instance cache. It actually might make more sense to have a shared cache here but I don't understand what's going on well enough to know.
- The cache holds a strong reference to the arguments of the function including the
selfargument so if you decorate the method directly then instances can be kept alive by the cache. This instead makes a reference loop but the gc will collect it.
It would probably be better to shift this to a top level or static method and use a shared cache.
There was a problem hiding this comment.
I guess the main concern is that the font objects appear to be mutable. I wonder if there is a good way to copy them so we can avoid mutating cache entries...
There was a problem hiding this comment.
I see. Thanks for the explanation!
|
What's the status of this PR? |
|
I don't really understand matplotlib, so it's difficult to judge whether the patch is correct or not and I don't have that much faith in our test coverage. But it seems okay so we could just merge it and see if it unblocks the update. It would be nice to upstream some of the stuff but we have nobody who either understands matplotlib or wants to spend time learning about it. So it might be best just to wait until someone shows up with the inclination to maintain this. |
|
I think we can merge and release? @hoodmane |
4 participants
Matplotlib removed maxdict:
https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.6.0.html#miscellaneous-internals
I asked about this on the matplotlib discourse:
https://discourse.matplotlib.org/t/matplotlib-pyodide-font-caching-with-maxdict-and-lru-cache/24260